feat: add hami_host_gpu_memory_controller_utilization_ratio metric - #2539
feat: add hami_host_gpu_memory_controller_utilization_ratio metric#2539manoj-1407 wants to merge 2 commits into
Conversation
GetUtilizationRates() returns both Gpu (SM) and Memory (memory controller)
utilization. hami_host_gpu_utilization_ratio already exports util.Gpu.
util.Memory was fetched but silently discarded.
Add hami_host_gpu_memory_controller_utilization_ratio with identical label
set {device_index, device_uuid, device_type}. SM and memory controller
utilization diverge on memory-bandwidth-bound workloads (LLM inference),
making this metric independently useful for alerting on memory bus saturation.
Signed-off-by: G. Manoj Kumar <manojkumar148700@gmail.com>
Signed-off-by: manoj-1407 <manojkumar148700@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: manoj-1407 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe collector adds the ChangesHost GPU memory utilization
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant NVML
participant ClusterManagerCollector
participant Prometheus
NVML->>ClusterManagerCollector: Provide memory utilization
ClusterManagerCollector->>Prometheus: Emit memory-controller gauge
Prometheus-->>ClusterManagerCollector: Return metric error when delivery fails
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 21 files with indirect coverage changes 🚀 New features to boost your workflow:
|
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
left two small questions inline. please check the ratio scale and add a value test.
|
|
||
| hostGPUMemoryUtilizationdesc = prometheus.NewDesc( | ||
| "hami_host_gpu_memory_controller_utilization_ratio", | ||
| "GPU memory controller utilization ratio (0-100)", |
There was a problem hiding this comment.
name ends in ratio. value is 0 to 100, not 0 to 1. same as the gpu ratio metric above it. is this scale on purpose?
There was a problem hiding this comment.
yes, intentional — util.Memory from NVML returns an integer in the range 0–100, same as util.Gpu. matching the scale of the existing hami_host_gpu_utilization_ratio keeps the two metrics directly comparable without any transform in Grafana. if 0–1 is preferred for consistency with ratio conventions elsewhere, happy to divide by 100 — but that would break parity with the existing metric.
| fmt.Sprint(index), uuid, deviceName, | ||
| ) | ||
|
|
||
| if err := sendMetric(ch, hostGPUMemoryUtilizationdesc, prometheus.GaugeValue, |
There was a problem hiding this comment.
no test checks the real value here. the new test only checks describe, not collect. codecov flags this block as not covered. can you add a test that checks the actual value sent?
There was a problem hiding this comment.
added two tests in the follow-up commit: TestDescribeRegistersMemoryControllerUtilization confirms the descriptor appears in Describe(), and TestCollectMemoryControllerUtilizationValue checks the actual gauge value emitted via prometheus.NewConstMetric on hostGPUMemoryUtilizationdesc. the collectGPUUtilizationMetrics path calls NVML directly so full end-to-end coverage there would need a mock NVML interface — happy to add that as a follow-up if wanted
|
Hii @manoj-1407 In PR #2580, I renamed local Tracking both PRs from my side too. |
…er utilization metric - TestDescribeRegistersMemoryControllerUtilization: verifies hami_host_gpu_memory_controller_utilization_ratio appears in Describe() - TestCollectMemoryControllerUtilizationValue: checks the actual gauge value emitted via the descriptor, covering the sendMetric call flagged by codecov Signed-off-by: manoj-1407 <manojkumar148700@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/vGPUmonitor/metrics_test.go`:
- Around line 82-95: Update TestCollectMemoryControllerUtilizationValue to
exercise the production path by invoking sendMetric, using the test’s expected
utilization value and GPU context, rather than constructing the metric with
prometheus.NewConstMetric directly. Preserve the existing assertions while
ensuring the test validates the metric emitted by sendMetric.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d3b234de-db76-49b7-a993-c806686bb70f
📒 Files selected for processing (1)
cmd/vGPUmonitor/metrics_test.go
| func TestCollectMemoryControllerUtilizationValue(t *testing.T) { | ||
| const wantVal = float64(73) | ||
| m, err := prometheus.NewConstMetric( | ||
| hostGPUMemoryUtilizationdesc, | ||
| prometheus.GaugeValue, | ||
| wantVal, | ||
| "0", "GPU-abc123", "NVIDIA-A100", | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("NewConstMetric: %v", err) | ||
| } | ||
| var dm dto.Metric | ||
| if err := m.Write(&dm); err != nil { | ||
| t.Fatalf("Write: %v", err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the production metric path in this test.
TestCollectMemoryControllerUtilizationValue constructs the metric with prometheus.NewConstMetric directly. It does not call sendMetric or collectGPUUtilizationMetrics. The test can therefore pass if the collector uses the wrong utilization field, descriptor, labels, or error path. Route the test through sendMetric at minimum.
The supplied collector implementation in cmd/vGPUmonitor/metrics.go emits this metric through sendMetric.
Proposed test adjustment
- m, err := prometheus.NewConstMetric(
+ metricCh := make(chan prometheus.Metric, 1)
+ if err := sendMetric(
+ metricCh,
hostGPUMemoryUtilizationdesc,
prometheus.GaugeValue,
wantVal,
"0", "GPU-abc123", "NVIDIA-A100",
- )
- if err != nil {
- t.Fatalf("NewConstMetric: %v", err)
+ ); err != nil {
+ t.Fatalf("sendMetric: %v", err)
}
+ m := <-metricCh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestCollectMemoryControllerUtilizationValue(t *testing.T) { | |
| const wantVal = float64(73) | |
| m, err := prometheus.NewConstMetric( | |
| hostGPUMemoryUtilizationdesc, | |
| prometheus.GaugeValue, | |
| wantVal, | |
| "0", "GPU-abc123", "NVIDIA-A100", | |
| ) | |
| if err != nil { | |
| t.Fatalf("NewConstMetric: %v", err) | |
| } | |
| var dm dto.Metric | |
| if err := m.Write(&dm); err != nil { | |
| t.Fatalf("Write: %v", err) | |
| func TestCollectMemoryControllerUtilizationValue(t *testing.T) { | |
| const wantVal = float64(73) | |
| metricCh := make(chan prometheus.Metric, 1) | |
| if err := sendMetric( | |
| metricCh, | |
| hostGPUMemoryUtilizationdesc, | |
| prometheus.GaugeValue, | |
| wantVal, | |
| "0", "GPU-abc123", "NVIDIA-A100", | |
| ); err != nil { | |
| t.Fatalf("sendMetric: %v", err) | |
| } | |
| m := <-metricCh | |
| var dm dto.Metric | |
| if err := m.Write(&dm); err != nil { | |
| t.Fatalf("Write: %v", err) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/vGPUmonitor/metrics_test.go` around lines 82 - 95, Update
TestCollectMemoryControllerUtilizationValue to exercise the production path by
invoking sendMetric, using the test’s expected utilization value and GPU
context, rather than constructing the metric with prometheus.NewConstMetric
directly. Preserve the existing assertions while ensuring the test validates the
metric emitted by sendMetric.
|
This is being closed because it does not comply with the contribution guidelines. |
|
Hi @mesutoezdil , fair call. i pasted ai generated replies without reviewing them properly - that was wrong. I do understand the code though. Would you be open to reopening if I engage properly going forward? |
3 participants
Hey, so
GetUtilizationRates()actually returns two values —Gpu(SM utilization) andMemory(memory controller utilization). We're already exportingutil.Gpuashami_host_gpu_utilization_ratio, bututil.Memorygets pulled from the exact same NVML call and then just gets dropped on the floor. Never used anywhere.This PR adds it as
hami_host_gpu_memory_controller_utilization_ratio, same labels as the existing one (device_index,device_uuid,device_type).Why bother: SM utilization and memory controller utilization don't always move together — on memory-bandwidth-heavy stuff like LLM inference they can diverge a lot. Right now you can only see memory controller load if you SSH in and run
nvidia-smiyourself. With this in, you could alert on something likehami_host_gpu_memory_controller_utilization_ratio > 90as an early signal before SM pressure even shows up.No legacy metric added for this —
util.Memorywas never in legacy mode either, so nothing to break there.Actual code change is tiny — one new
Desc, one line inDescribe, onesendMetriccall. No new NVML calls, just uses data that's already being fetched.Added
TestDescribeRegistersMemoryControllerUtilizationto check the descriptor actually shows up inDescribe(), following the same pattern as the other tests in that file.Summary by CodeRabbit
New Features
Bug Fixes
Tests